Make ModifierComparer use type references instead of definition #7557
Conversation
When comparing definitions this was failing if the type couldn't be resolved, for instance if a type-forwarded dependency was missing or references were specified at all. This was inconsistent with our other parameter comparisons which just evaluate the compare the type reference as a string without considering assembly. This does raise a concerning issue, that APICompat doesn't actually evaluate assembly equivalence of parameters but that's an existing problem.
There was a problem hiding this comment.
Looks like this is my bad because I added ModifierComparer in d6dd7a0#diff-ab264297f1e0d7296c8d69384a224e589d95ced59d203663d968dcc20f7d1e13R131
I'm slightly concerned this isn't the only instance of the problem because I did a lot of copying and pasting in #4585 Pretty sure I copied most of that class from somewhere else but have no idea where…
| @@ -144,7 +144,7 @@ public bool Equals(ICustomModifier x, ICustomModifier y) | |||
| } | |||
|
|
|||
| return x.IsOptional == y.IsOptional && | |||
| TypeComparer.Equals(x.Modifier.ResolvedType, y.Modifier.ResolvedType); | |||
| TypeComparer.Equals(x.Modifier, y.Modifier); | |||
There was a problem hiding this comment.
Does this allow both ICustomModifiers to be unresolved but disallow one or the other (and not both) to be resolved❔
There was a problem hiding this comment.
It's just using the type-reference to do the comparison so it's irrelevant if either type is/can be resolved. This is consistent with how we already compare parameter types.
|
@dougbu you did a solid by fixing the original issue, this just tweaks your fix to not require TypeDefinitions to be equal. I think this is the only place you introduced that's doing TypeDefinition comparisons. I didn't see any other places where we were doing this (looking for uses of |
When comparing definitions this was failing if the type couldn't be resolved,
for instance if a type-forwarded dependency was missing or references
were specified at all.
This was inconsistent with our other parameter comparisons which just
evaluate the compare the type reference as a string without considering
assembly.
This does raise a concerning issue, that APICompat doesn't actually
evaluate assembly equivalence of parameters but that's an existing
problem.
Fixes #2102, #7511.
Related #2136
cc @utpilla